Repository navigation
fix(core): wrap observer-object handlers like functions, and let a wrapped promise settle after teardown - #3829
armando-navarro wants to merge 4 commits into
Conversation
Firebase accepts a subscription handler either as a function or as an object with next, error and complete members (data-connect spells them onNext, onErr, onComplete). The zone wrapper only recognized the function, so an observer object reached the SDK untouched: no pending task held server rendering, inject() threw inside the handler, and in a zone.js app the handler ran outside the Angular zone, where the wrapper itself had moved the call. The object is now replaced by a copy whose function members are wrapped the same way, with this bound to the caller's object. onConfigUpdate takes only the object form and its handler fires only on a remote template change, so it joins the blockUntilFirst: false list along with onTokenChanged, whose first callback a first visit may never request. Both would otherwise hold the application unstable forever once their observers are wrapped.
When a wrapped call returns a Promise, the wrapper resolved its own promise through runInInjectionContext and NgZone.run. Neither reaches the caller: a .then callback runs as a later microtask after the injection context is torn down, and zone.js runs it in the zone where .then was attached, not where resolve was called. The only effect the two layers had was on teardown. runInInjectionContext throws once its injector is destroyed, so a Firebase promise settling after ApplicationRef.destroy() or a TestBed reset threw before resolve ran and the caller's promise never settled. The pending task still opens on the call and closes on settlement.
tyler-reitz
left a comment
There was a problem hiding this comment.
Approving. A fresh build regenerates both files byte-identical to the PR, Chrome headless passes 140/140, and rebuilding with main's src/zones.ts fails exactly the 14 zones.spec.ts cases the body names.
Non-blocking nits:
- The PR title is
fix:but both commits usefix(core):, which is what the squash should carry. Worth retitling before merge. src/zones.tsadds/* */comments among the existing//ones. The content is worth keeping; only the style differs.isObserverLikereads members withReflect.get, so it also matches anext,errororcompleteinherited through the prototype, and any wrapped call's object argument that has one gets copied. I don't know of a Firebase argument type like that, but I haven't audited every wrapped signature, so this is a question rather than a finding.
The previous commit treated any object argument with a next, error or complete method as an observer. A model class with a complete() method, saved through setDoc with a converter, was replaced by a plain copy that lost its prototype, and its pending task was never released. tools/build.ts now reads Firebase's type declarations with the TypeScript compiler API and passes a fourth ɵzoneWrap argument, true, only to the 11 exports with an observer-shaped parameter. Every other function receives its object arguments untouched. A handler Firebase calls after the app is destroyed now runs directly, because entering a destroyed injector throws NG0205. Handlers passed as functions already hit this on main.
# Conflicts: # src/app-check/firebase.ts # src/remote-config/firebase.ts # src/zones.spec.ts # tools/build.ts
|
Thanks for the review, @tyler-reitz. Your third point turned up a bug this PR introduced, so the branch has changed since your approval. Can you take another look? TitleRetitled to Comment styleThe
|
Fixes #3828
ɵzoneWrapnow wraps a handler passed as an observer object the way it wraps a handler passed as a function, for the 11 functions whose declared parameters include an observer. Firebase receives a copy of the object whosenext,error,complete,onNext,onErrandonCompletemembers are wrapped and bound to the caller's object.tools/build.tsfinds those 11 at build time by reading Firebase's type declarations with the TypeScript compiler API, and passes them a fourthɵzoneWrapargument,true. Every other wrapped function passes its object arguments through untouched.onConfigUpdate(remote-config) getsblockUntilFirst: false, because wrapping its observer attruewould hold the application unstable until a config update a normal load may never produce.runInInjectionContextandNgZone.runlayers that never reached the caller and threw once the injector was destroyed.NG0205, in both forms.Checklist
ɵzoneWrapignores a handler passed as an observer object, and holds a wrapped promise forever once its injector is destroyed #3828docs/zones.md, saying the re-entry holds for a handler passed as an object too, goes in the rc.1 docs PR, docs: document the 22.0.0-rc.1 changes #3835.src/zones.spec.tshas fifteen cases, each run underprovideZoneChangeDetection()and underprovideZonelessChangeDetection().src/firestore/firestore.spec.tshas one more, against the realsetDocand the emulator.yarn install,yarn testrun successfully?: yes, with npm:npm ci,npm run build,npm test(which runsbuild:jasmine,test:node-esmandtest:chrome-headless), plusnpm run test:nodeandnpm run lint.Changes
Before this change the argument loop in
ɵzoneWraptestedtypeof _arguments[i] === 'function'and nothing else, so an observer object reached the SDK untouched. The issue lists the eleven affected functions and what each form lost.The first commit:
isObserverLike, true for a non-null object with a function under any of the six member names, andzoneWrapObserver, which returns a copy of the object with those members replaced by the same wrapper a function argument gets..bind(observer)keepsthisfor a class instance or an RxJSSubject.onConfigUpdate: { blockUntilFirst: false, logLevel: LogLevel.WARN }to the remote-config block oftools/build.ts.onTokenChanged, which fix: stop twelve wrapped calls from holding the app unstable #3827 also added onmain, so after the merge only theonConfigUpdateentry is new.The second commit:
ret.then((it) => runInInjectionContext(injector, () => run(() => resolve(it))), ...)withret.then(resolve, reject).assertNotDestroyedcheck is what hung a promise settling after teardown (the issue has the details).The third commit:
complete()method, saved throughsetDocwith a converter, reached the converter as a plain copy without its prototype, and the call's pending task was never released.exportsTakingAnObservertotools/build.ts. It type-checks an in-memory file that imports each Firebase entry point, and keeps the exports with a parameter whose type is an object with a function-valued observer member.ɵzoneWrap(fn, blockUntilFirst, logLevel, true).ɵzoneWraplooks for an observer object only when that fourth argument istrue.zoneWrapFncall the handler directly wheninjector.destroyedis true. Before, a handler Firebase called afterApplicationRef.destroy()or aTestBedreset threwNG0205, which already happened onmainfor handlers passed as functions.The merge of
main:isSupportedentry and this PR'sonConfigUpdateentry.src/zones.spec.ts, which both sides created, keeps both sets of specs.What callers can notice
onSnapshot(ref, { next })and the other object-form calls now hold server rendering until the first value, run their members inside the Angular zone, and can callinject()inside them, exactly like the function form.onConfigUpdate, which never held the application unstable (its only form was never wrapped), does not start to now that its observer is.setDocincluded, receives its object arguments exactly as before this PR.inject()inside it are the same as before.NG0205.Verification
injector.destroyedcheck fromzoneWrapFnfails the new teardown spec in both modes withNG0205: Injector has already been destroyed.ɵzoneWrapfails five specs: the two data-object cases in both modes, and the FirestoresetDocconverter spec.npm run test:nodeandnpm run test:node-esm: 401 specs, 0 failures each.npm run lintpasses.@angular/fire/firestoreand@angular/fire/remote-configentry points: the object form matches the function form on pending task, zone andinject()in both apps, andonConfigUpdateregisters no task. This was rerun with the build-time flag in place, before theNG0205change and the merge.getDocregisters and releases its task and resolves with the document, a rejecting wrapped call releases its task and the rejection reaches the caller, the caller's continuation sees the same zone and the sameinject()failure as before, and a wrapped call made inside a childEnvironmentInjectorthat is destroyed before the promise settles resolves.Backport
Non-breaking.
src/zones.tson21.1.xis identical to this branch's starting point. A cherry-pick PR against21.1.xfollows at a later date, after a run on an Angular 21 app.